MPDX-9818 - Add DA5 and A33 warnings/errors to goal settings - #1930
MPDX-9818 - Add DA5 and A33 warnings/errors to goal settings#1930zweatshirt wants to merge 4 commits into
Conversation
zweatshirt
added
Preview Environment
|
Preview branch generated at https://MPDX-9818.d3dytjb8adxkk5.amplifyapp.com |
Bundle sizes [mpdx-react]Compared against ea57016
|
zweatshirt
left a comment
zweatshirt
left a comment
There was a problem hiding this comment.
Multi-Agent Code Review — PR #1930
Verdict: APPROVED WITH SUGGESTIONS — no blockers. Highest finding is 6.5/10.
Risk: 7/10 (HIGH) — src/components/**/*.graphql (+2), 4x feature components (+4), 146 non-test lines (+1). Senior review recommended. Codegen verified clean (yarn gql succeeds, yarn lint:ts exits 0, 213 tests pass).
Two rounds: 6 specialized agents each. Round 2 was re-run with round-1 findings adjudicated against verified facts, which recalibrated three findings downward and surfaced two better ones.
Key recalibrations from round 1
| Finding | Was | Now | Why |
|---|---|---|---|
| Failed preview silently suppresses warning | 7.5 | 3.5 | Self-corrects on save (UpdateNewStaffGoalCalculation returns the fragment incl. salaryOverCap; enableReinitialize set; Save never gated on it) |
| Duplicate preview mutations | 7.5 | 6.5 | BatchHttpLink (batchInterval 20ms) coalesces both into one HTTP request; cost is doubled server compute, not round-trips |
savedSalaryOverCap = false fails open |
4.5 | 2.0 | The only caller omitting it (MpdGoalPreview) never reads salaryOverCap |
Verified correct (no action)
- The always-mounted empty
aria-liveregion is proper live-region technique — mutation-tested: deletingaria-liveand making the region conditional both fail the suite. - Cache normalization correct (
NewStaffGoalCalculationCalculationshas noid; non-normalizable value type).fetchPolicy: 'no-cache'correct (NewStaffGoalCalculationPreviewis a distinct typename). - Destructuring-with-defaults change is behaviourally identical (
monthlyGoal: Float!,salaryOverCap: Boolean!). - Scenario-goal path (
accountListId: null) is valid per schema and pre-existing viaMpdGoalPreview. - Save-while-preview-in-flight race traced and safe.
- Cap semantics: a non-joining senior-staff spouse's salary IS included against the married cap (
applicable? = !spouse? || !single?) — coherent, no bug. SOSA correctly treated as single. - Mock fixtures deterministic (
seed = 'seed', probed across processes). salaryOverCap: falseadded toMpdGoalPreview.test.tsxis inert — removing it still passes 15/15.- Missing
public/locales/enkey follows the repo's batch-extraction convention. Not a violation.
Findings outside this PR's diff (informational, not blockers)
1. The contrast fix belongs in src/theme.ts, not in this component. src/theme.ts never defines palette.warning, so warning.main resolves to MUI's default #ed6c02 (3.11:1 on #fff). But HrTools uses warning.main in 7 of 7 pre-existing cases — changing only this file would make it the sole diverging file. Systemic fix:
warning: { main: statusColors.warning }, // #D34400 -> 4.57:1
success: { main: statusColors.success }, // CardTitle.tsx:24 has the same latent defectPre-existing HrTools-wide. Recommend a standalone ticket rather than blocking this PR.
2. Review-process gap. .claude/rules/code-review.md lists src/components/User/impersonate*, which matches nothing — impersonation lives in Settings/Admin/ImpersonateUser/, Settings/Organization/ImpersonateUser/, and ProfileMenu/. The Security agent's impersonation trigger never fires. Also stale: src/components/Reports/SalaryCalculator/** and pages/api/Schema/uploads/** don't exist.
3. Doc drift. CLAUDE.md and the rules file say "Material UI v5"; yarn.lock resolves @mui/material 7.3.11.
4. Questions for the API team (mpdx_api #3469):
salary_over_cap?callsbase_salary_capunconditionally, whereassalary_caponly reaches it whenallow_salary_over_cap == 'no'. It resolves viafind_by!, which raises. Are the fourboard_approved_salary_calcmisc constants seeded for every historicalcalculations_year? A raise would null the wholecalculationsblock via non-null propagation, takingmonthlyGoaldown with it.salary_over_cap?uses strict>— confirm equal-to-cap is intended as compliant.
Recommended order of work
- The flicker fix (
useMpdGoalPreview.ts:151) — one line, real user-facing defect - Two test assertions (warning renders in the form;
accountListIdin both directions) — closes a mutation-proven hole - Resolve the dead
mocksprop either way - Copy revision
- Defer: theme palette, doc drift, rules globs
zweatshirt
left a comment
zweatshirt
left a comment
There was a problem hiding this comment.
Multi-Agent Code Review — PR #1930 (Round 3)
Verdict: APPROVED WITH SUGGESTIONS — no blockers. Highest finding 6.0/10.
Risk: 7/10 (HIGH) — src/components/**/*.graphql (+2), 4x feature components (+4), ~145 non-test lines (+1). Senior review recommended.
6 agents (Architecture, Testing, Standards, Data Integrity, UX, Financial Reporting). Third round on this branch; the six fix-up commits since the last review closed every prior finding, so this pass focuses on the new code.
Confirmed fixed since the last review
Verified by mutation testing — each item below now fails a targeted test when reverted:
| Fix | Guarded by |
|---|---|
| Warning flicker on every edit | does not re-raise a resolved warning… |
accountListId untested in both directions |
both useNewStaffGoalCalculation tests |
| Warning-in-form integration unasserted | shows the over-cap warning beside the save actions |
| Real timers against a 500ms debounce | fake timers added; probed with a 5,000,000ms debounce |
Dead mocks test prop |
removed |
Stale plural JSDoc, unused export |
fixed |
Mutation audit: 9 of 10 mutations caught. The one gap is the .catch fallback — see the inline comment on useMpdGoalPreview.ts.
Root cause found for "the warning shows when Annual Requested Salary is empty"
This was reported during review and is not stale persistence. app/services/new_staff_goals/salary_calculation.rb:73-81:
requested_annual_salary = calculator_attribute(:requested_annual_salary)&.to_d
requested_annual_salary || ((geographic_multiplier * base) + tenure_adjustment + debt_salary)An empty field maps to null on the client (toNumberOrNull('')), and the API then computes a default salary and compares that against the cap. The flag is correct — the goal calculation really does use that computed figure. The copy is what's wrong: it says "requested salary" when nothing was requested. See the inline comment on GoalSettingsWarning.tsx.
Findings on related files (not in this PR)
1. Contrast fix belongs in src/theme.ts, not in the component. palette.warning is never defined, so warning.main resolves to MUI's default #ed6c02 — 3.11:1 on #fff, failing the 4.5:1 bar for 14px body2. The repo's own statusWarning (#D34400) is 4.57:1. But HrTools uses warning.main in 7 of 7 pre-existing cases, so changing only this file would make it the sole outlier. Systemic fix:
warning: { main: statusColors.warning }, // #D34400 -> 4.57:1
success: { main: statusColors.success }, // CardTitle.tsx:24 has the same latent defectPre-existing and tree-wide — should not block this PR. Standalone ticket recommended.
2. Review-process gap. .claude/rules/code-review.md lists src/components/User/impersonate*, which matches nothing — impersonation lives under Settings/Admin/ImpersonateUser/, Settings/Organization/ImpersonateUser/, and ProfileMenu/. The Security agent's impersonation trigger never fires. Also stale: src/components/Reports/SalaryCalculator/** and pages/api/Schema/uploads/**.
3. Doc drift. CLAUDE.md and the rules file say "Material UI v5"; yarn.lock resolves @mui/material 7.3.11.
Questions for the API team (mpdx_api #3469)
- Expose the amounts behind the warning. To say "Total salary of $X is over the standard cap of $Y" the client needs
combined_uncapped_total_salaryandbase_salary_caponNewStaffGoalCalculationCalculations. Neither is exposed today, and the three salary fields that are (salary,salarySubtotal,totalSalary) are all monthly and post-capping —base_salaryscales down when the combined total exceeds the cap, sototalSalaryis by construction never over the cap. Rendering any of them beside "is over the cap" would show a number that isn't. This is the blocker on improving the copy further. - Seeding.
salary_over_cap?callsbase_salary_capunconditionally, whereassalary_caponly reaches it whenallow_salary_over_cap == 'no'. It resolves viaMpdGoalMiscConstant.find_by!, which raises, and a raise would null the wholecalculationsblock via non-null propagation — takingmonthlyGoalwith it. The in-repo seeder is all-or-nothing per year (so the risk is pre-existing, not new), but production seeding isn't in the repo. Is the prod path also all-or-nothing per year, and is everycalculations_yearin use backfilled? - Seeded cap values differ from the spec.
MpdGoalMiscConstantsSeederhas 80k/125k/90k/145k;goal_calculator_spec.rbstubs 85k. Which figures are current?
| setPreview({ | ||
| attributes: debounced, | ||
| monthlyGoal: null, | ||
| salaryOverCap: null, |
There was a problem hiding this comment.
This .catch sets salaryOverCap: null, which line 152's ?? coalesces to savedSalaryOverCap. So an admin who raises the salary over cap, sees the warning, then makes any further edit whose preview request fails, watches the warning disappear — and can reasonably read that as "resolved". There is no snackbar (context: { suppressErrors: true }, line 108) and GoalSettingsWarning never consumes the hook's failed flag, so unlike MpdGoalPreview — which renders "Preview unavailable" for the same failure — the warning bar shows nothing at all.
This also contradicts the flicker fix's own principle: every other path holds the last known answer rather than reverting to saved.
Untested. Changing this null to false left 165 of 165 tests passing. It became unreachable from the suite when the mocks prop was removed from GoalSettingsWarning.test.tsx — that prop was the only handle for forcing a failure.
Two defensible resolutions:
// (a) hold the last known verdict rather than dropping it
.catch(() => {
if (active) {
setPreview((prev) => ({
attributes: debounced,
monthlyGoal: null,
salaryOverCap: prev?.salaryOverCap ?? null,
}));
}
});or (b) keep reverting to saved, but have GoalSettingsWarning render the failed state the way MpdGoalPreview does, so the disappearance is explained rather than silent.
Either way, add a test — re-introducing the mocks override (the pattern still present in MpdGoalPreview.test.tsx) is enough.
Severity is bounded because the warning is advisory, never gates Save, and self-corrects on the next successful preview or on save.
| savedSalaryOverCap = false, | ||
| previewSalaryOverCap = false, | ||
| }) => ( | ||
| <GqlMockedProvider<{ |
There was a problem hiding this comment.
11 test files in this directory wrap in <ThemeProvider theme={theme}>, including the closest analogue MpdGoalPreview.test.tsx:56-58.
GoalSettingsWarning.tsx:35-36 styles on warning.main, so in tests it resolves against MUI's default palette rather than src/theme. The suite therefore cannot catch a token that exists in the default theme but not the project's — which is precisely the situation here (palette.warning is undefined in src/theme.ts, see the review body).
Low blast radius, but it's the reason the contrast issue was invisible to CI.
|
@zweatshirt Are you also going to add warnings that the user is healthcare exempt, SECA exempt, or over the debt cap? (Sorry I missed the debt cap before with the API PR. I haven't dug in, but that check may need to happen on the server.) |
|
@canac I can add that to this PR. My brain assumed we weren't adding them due to what we concluded with the backend PR, but it makes sense we want them still. |
|
Yesterday, I was just thinking about the |
|
@canac The backend PR added a boolean for healthcare exemption but you mentioned we don't need to check for it and so I removed it, are we still wanting to implement it? |
|
The sheet just surfaces a non-blocking warning when someone has an opt out or exemption. I think we can just read the booleans from the goal calculation and show a message if any of them are true. Like it says something like "SECA Opted Out (Both)", for example. I don't think that needs to be a validation in the backend calculations. What do you think? |
|
@canac That was what I was intending with the boolean in the GraphQL for healthcare exemption. Is the goal to do as much of the logic for the warnings in the the frontend then? My idea was that we could send booleans to the frontend on which warnings to display and have the frontend act on those (or in the case of SECA we could maybe do an enum) I guess the frontend has enough information for most of these apart from the salary cap and debt cap to do it in the frontend instead, though |
|
@canac This is basically ready for review. I've done multiple review agent iterations, it is considered critical. Sorry, it turned out to be a much bigger PR than I expected. Feel free to share constructive criticism. |
zweatshirt
changed the title
|
@canac Did you want me to redo this PR? I noticed it hadn't been reviewed in a while. It ended up being a big PR, and there are always areas to improve, so I am happy to rework it if needed. |
|
Oh sorry, for some reason I thought you were going to make more changes. I can look over it tomorrow if it's ready for review! |
zweatshirt
left a comment
zweatshirt
left a comment
There was a problem hiding this comment.
🤖 Multi-Agent Code Review — ✅ Approved with Suggestions
6 specialized reviewers (Architecture, Testing, Standards, Data Integrity, UX, Financial-domain) + dependency analysis. All returned High confidence with zero critical / high / important findings. What remains is a set of optional low-severity test-coverage and a11y-polish suggestions.
Scope note: this review deliberately excludes local commit
e67821d21(review-process / governance files) per the author's request. The reviewed tree is the remote PR headbb84087ac— the 16 goal-settings feature files — so it matches this PR's diff exactly.
Risk: 🟢 LOW
| Axis | Score | Why |
|---|---|---|
| Blast Radius | 1 | Contained to GoalSettings/; one additive .graphql fragment (0 external dependents on every changed export) |
| Complexity | 2 | New warning rule-engine + shared preview state held across in-flight edits |
| Sensitivity | 1 | Displays financial figures; .graphql floor. No auth / PII / money-write |
| Recoverability | 0 | CI-gated frontend, fully revertible, no persisted side effects |
effort 3 · danger 1 → auto-approve eligible.
Dependency impact
Blast radius genuinely contained. useMpdGoalPreview, MpdGoalPreview, GoalSettingsHeader, useGoalSettingsField, and all new exports have 0 external dependents; every in-directory call site was updated. The only cross-boundary change — adding salaryOverCap / debtOverCap to NewStaffGoalCalculationFields — is purely additive.
Highlights
- ✅ Data integrity sound: previewing (a
mutation) cannot drift the saved display — protected three ways (fetchPolicy: 'no-cache', distinctNewStaffGoalCalculationPreviewtypename, non-normalized value-objectcalculations). - ✅ N+1 removed: the shared provider collapses per-consumer preview requests into one (verified by "previews an edit exactly once with every consumer mounted").
- ✅ A11y choices are deliberate & correct:
aria-live="polite"+role="presentation"on the grouped alerts, and noaria-invalidon advisory outlines. - ✅ Money math safe: the single arithmetic line is the established cents-rounding idiom; no
-$0.00path.
Findings
8 open suggestions (all severity ≤ 5.0) posted inline. Nothing blocks merge.
Dismissed by author (acknowledged — not counted toward the verdict)
- useMpdGoalPreview.ts (3.5) — held over-cap flag / transient header-vs-warning divergence during in-flight edits — intentional, documented behavior
- GoalSettingsPreviewContext.tsx (3.0) — warnings rebuild each keystroke — intentional, documented behavior
Also: the prior sev-6.5 /dismissed finding ("every edit runs the goal calculation twice") is resolved by this PR's shared-provider refactor — no new finding matches it.
Security agent skipped (no api/auth/apollo-config/env/workflow/dependency changes in scope). Generated by a multi-agent review harness — findings are advisory.
| initialTouched={initialTouched} | ||
| onSubmit={jest.fn()} | ||
| > | ||
| <GoalSettingsPreviewProvider |
There was a problem hiding this comment.
| expect(getByText('Staff opted out of SECA')).toBeInTheDocument(); | ||
| expect(queryByText('Both opted out of SECA')).not.toBeInTheDocument(); | ||
| }); | ||
| it('announces warnings politely, from a region that exists while empty', () => { |
There was a problem hiding this comment.
| .filter(({ fields }) => fields.includes(name)) | ||
| .map(({ severity }) => severity); | ||
|
|
||
| return severities.find((severity) => severity === 'error') ?? severities[0]; |
There was a problem hiding this comment.
| return candidates.filter(({ active }) => active); | ||
| }; | ||
|
|
||
| export const getFieldSeverity = ( |
There was a problem hiding this comment.
| showLabel?: boolean; | ||
| }; | ||
|
|
||
| const outlineSx = (severity: WarningSeverity) => ({ |
There was a problem hiding this comment.
| <Alert | ||
| key={severity} | ||
| severity={severity} | ||
| role="presentation" |
There was a problem hiding this comment.
| const previewDebtOverCap = dirty ? (preview?.debtOverCap ?? null) : null; | ||
|
|
||
| return { calculating, displayGoal, diff, changed, failed }; | ||
| return { |
There was a problem hiding this comment.
| values.benefitsPlan !== MpdGoalBenefitsConstantPlanEnum.Select; | ||
|
|
||
| // Only reachable via `everyoneExempt`, which is either both spouses or a lone applicant | ||
| const staffName = values.firstName || t('Staff'); |
There was a problem hiding this comment.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Auto-Approval
Risk Level: LOW (0/10)
Verdict: APPROVED_WITH_SUGGESTIONS (suggestions posted, no blockers)
This PR was auto-approved because:
- The multi-agent AI review determined it is low risk
- No blocking issues were found
- All suggestions have been posted as review comments for the developer to consider
- 3 non-blocking finding(s) were reviewed and dismissed by the developer
If you believe this PR needs human review, dismiss this approval and request a review manually.
|
@canac I can finish up the code given the agent review (sorry, I know it's produced a lot of comments, I wanted to test the new review scoring here). If you are up for it, I was mostly hoping for a human look at the UI. |
canac
left a comment
canac
left a comment
There was a problem hiding this comment.
I don't really know what stakeholders want for validation, but this seems like a great start! Hopefully we can get more feedback at some point.
Highlighting the fields that contribute to the goal is a really cool idea!
| // Discrepancy with the sheet: DA5's plan list has no "Exempt" option, so it | ||
| // never defined this case. Following it literally makes Exempt a violating | ||
| // plan. Revisit with whoever owns the plan list if that reading is wrong. | ||
| const exemptPlanMismatch = |
There was a problem hiding this comment.
We don't need this check anymore on the frontend either.
| calculationId={calculation.id} | ||
| savedSalaryOverCap={calculation.calculations.salaryOverCap} | ||
| savedDebtOverCap={calculation.calculations.debtOverCap} |
There was a problem hiding this comment.
What if we passed the full calculation to the provider and then it can read whichever fields it needs?
| calculationId={calculation.id} | |
| savedSalaryOverCap={calculation.calculations.salaryOverCap} | |
| savedDebtOverCap={calculation.calculations.debtOverCap} | |
| calculation={calculation} |
| : Math.round((previewGoal - savedMonthlyGoal) * 100) / 100; | ||
| const changed = Math.abs(diff) >= CENT_EPSILON; | ||
| const previewSalaryOverCap = dirty ? (preview?.salaryOverCap ?? null) : null; | ||
| const previewDebtOverCap = dirty ? (preview?.debtOverCap ?? null) : null; |
There was a problem hiding this comment.
If we're adding more fields to the preview, maybe we should add the calculated 403b amounts too?
| 'contribution403bPercentage', | ||
| 'spouseContribution403bPercentage', | ||
| // Sets the cost-of-living multiplier, and so the cap itself. | ||
| 'geographicLocation', |
There was a problem hiding this comment.
We can leave this and see what stakeholders think if you want, but I'm thinking that we shouldn't mark this as invalid since it can't be changed (as long as it's correct). Age, tenure, and marital status also affect the salary cap, but we don't mark those fields as invalid.
2 participants
Description
We want to show a warning to the admin when the salary the admin has entered is over the salary cap.
Backend PR: https://github.com/CruGlobal/mpdx_api/pull/3469
Testing
Checklist:
/pr-reviewcommand locally and fixed any relevant suggestions